Skip to content

[rush] Fork npm-check into rush-lib#5390

Closed
cmalonzo wants to merge 11 commits intomicrosoft:mainfrom
cmalonzo:user/cmalonzo/npmaudit/replacenpmcheck
Closed

[rush] Fork npm-check into rush-lib#5390
cmalonzo wants to merge 11 commits intomicrosoft:mainfrom
cmalonzo:user/cmalonzo/npmaudit/replacenpmcheck

Conversation

@cmalonzo
Copy link
Contributor

@cmalonzo cmalonzo commented Oct 2, 2025 •
edited
Loading

Summary

The latest released version of npm-check has npm audit issues that we need to address. However, npm-check has not been updated in years. Therefore, this aims to fork npm-check into rush-lib so that we can use it locally.

Details

Open questions

  • should I put this in a separate project library outside of rush-lib?

Changes from npm-check

  1. Removed properties from state that were never being used or set, noted above INpmCheckState
  2. Remove peerDependencies off of INpmCheckPackageSummary as it was deprecated in npm-check and never set
  3. Remove emoji's support as it was never used
  4. Downgraded version of path-exist as latest version of path-exist is ESM
  5. Removed semverDiff as it was deprecated and moved into semver

How it was tested

  1. rush build -t rush-lib
  2. node apps/rush/lib/start.js upgrade-interactive
    See expected packages
image

Impacted documentation

@github-project-automation github-project-automation bot moved this to Needs triage in Bug Triage Oct 2, 2025
@cmalonzo
Copy link
Contributor Author

cmalonzo commented Oct 2, 2025

Addresses #5328

@cmalonzo cmalonzo force-pushed the user/cmalonzo/npmaudit/replacenpmcheck branch from 023387b to 4341f8a Compare October 2, 2025 17:21
@iclanton
Copy link
Member

iclanton commented Oct 3, 2025

This is dropping the original license and copyright, which need to be retained if this is a fork. However, it wouldn't really be appropriate to put that in this repo.

How much work would it be to rewrite the functionality that we need from npm-check from scratch?

@cmalonzo
Copy link
Contributor Author

cmalonzo commented Oct 3, 2025

@iclanton I'm working to gain clarity on the licenses now just FYI.

@iclanton iclanton moved this from Needs triage to In Progress in Bug Triage Oct 6, 2025
@iclanton iclanton changed the title Fork npm-check into rush-lib [rush] Fork npm-check into rush-lib Oct 6, 2025
@iclanton
Copy link
Member

iclanton commented Oct 6, 2025

@cmalonzo - This is also pulling in a bunch of dependencies. Would reimplementation be practical with fewer new dependencies?

@cmalonzo cmalonzo force-pushed the user/cmalonzo/npmaudit/replacenpmcheck branch from e3f11ac to 4394ca3 Compare October 6, 2025 22:20
@cmalonzo
Copy link
Contributor Author

cmalonzo commented Oct 6, 2025

@iclanton I've removed everything unused and kept only the functionality required for upgrade-interactive to work.

@dmichon-msft
Copy link
Contributor

dmichon-msft commented Oct 6, 2025

@TheLarkInn , can we move rush upgrade-interactive out of the core of Rush and put it in its own plugin or CLI tool? That'd mitigate this and users of the interactive upgrade tool would still have a path to accept the risk of stale dependencies if wanting to run the tool.

@TheLarkInn
Copy link
Member

TheLarkInn commented Oct 7, 2025 •
edited
Loading

@TheLarkInn , can we move rush upgrade-interactive out of the core of Rush and put it in its own plugin or CLI tool? That'd mitigate this and users of the interactive upgrade tool would still have a path to accept the risk of stale dependencies if wanting to run the tool.

Does this require users to manually install the upgrade-interactive plugin for users to use the feature? Or are we just instantiating it as a core feature but from a plugin?

I'm not crazy about that user experience personally.

@TheLarkInn
Copy link
Member

TheLarkInn commented Oct 7, 2025

@TheLarkInn , can we move rush upgrade-interactive out of the core of Rush and put it in its own plugin or CLI tool? That'd mitigate this and users of the interactive upgrade tool would still have a path to accept the risk of stale dependencies if wanting to run the tool.

@iclanton @octogonz What is your take regarding this too?

@octogonz
Copy link
Collaborator

octogonz commented Oct 7, 2025

If upgrade-interactive has lots of extra NPM dependencies, one benefit is that they would only need to be installed on demand when actually using the feature.

But it is a core feature, so if there are problems with the existing dependencies we should prioritize fixing that, not look for ways to procrastinate the work. 😊

@dmichon-msft
Copy link
Contributor

dmichon-msft commented Oct 7, 2025

Frankly I'd just suggest we make upgrade-interactive a standalone tool invoked via npx or whatever that just knows how to talk to a Rush repository.

iclanton
iclanton reviewed Oct 9, 2025
View reviewed changes
common/config/rush/browser-approved-packages.json
"name": "dependency-path",
"allowedCategories": [ "libraries" ]
},
{
Copy link
Member

@iclanton iclanton Oct 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These should all go in nonbrowser-approved-packages.json

@cmalonzo cmalonzo closed this Oct 17, 2025
@github-project-automation github-project-automation bot moved this from In Progress to Closed in Bug Triage Oct 17, 2025
@cmalonzo
Copy link
Contributor Author

cmalonzo commented Oct 17, 2025

Closing this PR in place of #5416

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iclanton iclanton iclanton left review comments

@octogonz octogonz Awaiting requested review from octogonz octogonz is a code owner

@apostolisms apostolisms Awaiting requested review from apostolisms apostolisms is a code owner

@D4N14L D4N14L Awaiting requested review from D4N14L

@dmichon-msft dmichon-msft Awaiting requested review from dmichon-msft dmichon-msft is a code owner

@patmill patmill Awaiting requested review from patmill

Assignees

No one assigned

Labels

None yet

Projects

Bug Triage
Status: Closed

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@cmalonzo @iclanton @dmichon-msft @TheLarkInn @octogonz